Skip to content

gh-120155: Add assertion to sre.c match_getindex()#120402

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:sre_sanity
Jun 12, 2024
Merged

gh-120155: Add assertion to sre.c match_getindex()#120402
vstinner merged 1 commit into
python:mainfrom
vstinner:sre_sanity

Conversation

@vstinner

@vstinner vstinner commented Jun 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Add an assertion to help static analyzers to detect that i*2 cannot overflow.

Example of Coverity issue on Python 3.12.2:

Error: INTEGER_OVERFLOW (CWE-125):
Modules/_sre/sre.c:2361:5: tainted_data_return: Called function "match_getindex(self, group)", and a possible return value may be less than zero.
Modules/_sre/sre.c:2361:5: assign: Assigning: "index" = "match_getindex(self, group)".
Modules/_sre/sre.c:2368:5: overflow: The expression "index * 2L" is considered to have possibly overflowed.
Modules/_sre/sre.c:2368:5: overflow: The expression "index * 2L + 1L" is deemed overflowed because at least one of its arguments has overflowed.
Modules/_sre/sre.c:2368:5: deref_overflow: "index * 2L + 1L", which might have underflowed, is passed to "self->mark[index * 2L + 1L]".
  2366|
  2367|       /* mark is -1 if group is undefined */
  2368|->     return self->mark[index*2+1];
  2369|   }

@vstinner
pythongh-120155: Add assertion to sre.c match_getindex()
2d2c3ec 
Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
@bedevere-app bedevere-app Bot mentioned this pull request Jun 12, 2024
Closed
@vstinner vstinner added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes and removed awaiting core review labels Jun 12, 2024
@vstinner

vstinner commented Jun 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@serhiy-storchaka @sobolevn: Would you mind to review this change?

I'm not sure that it's enough to fix static analyzers, but it helps me at least to detect that it's a false positive :-)

@vstinner

vstinner commented Jun 12, 2024

Copy link
Copy Markdown
Member Author

cc @stratakis

sobolevn
sobolevn approved these changes Jun 12, 2024
View reviewed changes

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

#if SIZEOF_SIZE_T > 4
# define SRE_MAXGROUPS ((SRE_CODE)INT32_MAX / 2)
#else
# define SRE_MAXGROUPS ((SRE_CODE)PY_SSIZE_T_MAX / SIZEOF_VOID_P / 2)

Definion of SRE_MAXGROUPS ^^^

It looks like that i cannot be realistically bigger than SRE_MAXGROUPS 👍

@vstinner

vstinner commented Jun 12, 2024

Copy link
Copy Markdown
Member Author

It looks like that i cannot be realistically bigger than SRE_MAXGROUPS 👍

_validate_outer() checks that groups <= SRE_MAXGROUPS.

@vstinner vstinner merged commit 42b25dd into python:main Jun 12, 2024
@vstinner vstinner deleted the sre_sanity branch June 12, 2024 13:27
@miss-islington-app

miss-islington-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 12, 2024
@vstinner @miss-islington
pythongh-120155: Add assertion to sre.c match_getindex() (pythonGH-12…
af30564 
…0402)

Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
(cherry picked from commit 42b25dd)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 12, 2024
@vstinner @miss-islington
pythongh-120155: Add assertion to sre.c match_getindex() (pythonGH-12…
c4417cc 
…0402)

Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
(cherry picked from commit 42b25dd)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-app

bedevere-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

GH-120409 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 12, 2024
@bedevere-app

bedevere-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

GH-120410 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jun 12, 2024
vstinner added a commit that referenced this pull request Jun 12, 2024
@miss-islington @vstinner
[3.12] gh-120155: Add assertion to sre.c match_getindex() (GH-120402) (…
ab30934 
…#120410)

gh-120155: Add assertion to sre.c match_getindex() (GH-120402)

Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
(cherry picked from commit 42b25dd)

Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner added a commit that referenced this pull request Jun 12, 2024
@miss-islington @vstinner
[3.13] gh-120155: Add assertion to sre.c match_getindex() (GH-120402) (…
e048092 
…#120409)

gh-120155: Add assertion to sre.c match_getindex() (GH-120402)

Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
(cherry picked from commit 42b25dd)

Co-authored-by: Victor Stinner <vstinner@python.org>
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@vstinner @mrahtz
pythongh-120155: Add assertion to sre.c match_getindex() (python#120402)
49d9a9b 
Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@vstinner @noahbkim
pythongh-120155: Add assertion to sre.c match_getindex() (python#120402)
47d4cb4 
Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@vstinner @estyxx
pythongh-120155: Add assertion to sre.c match_getindex() (python#120402)
0fffa6c 
Add an assertion to help static analyzers to detect that i*2 cannot
overflow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@sobolevn sobolevn sobolevn approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vstinner @serhiy-storchaka @sobolevn